Fix tests on macOS #184
Fix tests on macOS #184
Conversation
WalkthroughThe changes primarily focus on platform-specific adjustments for macOS, particularly in floating-point comparisons and LAPACK library usage. The CI/CD pipeline is simplified, and test execution steps are streamlined. The codebase now incorporates LAPACK functionality for macOS, and the test assertions have been updated to use tolerance checks for floating-point comparisons. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (9)
- .github/workflows/ci.yml (2 hunks)
- float/float64/ge_test.v (2 hunks)
- iter/ranges_test.v (3 hunks)
- la/matrix_ops_test.v (1 hunks)
- vlas/cflags_d_cblas.v (1 hunks)
- vlas/cflags_notd_cblas.v (1 hunks)
- vlas/lapack_common.v (2 hunks)
- vlas/lapack_default.c.v (1 hunks)
- vlas/lapack_macos.c.v (1 hunks)
Files skipped from review due to trivial changes (5)
- .github/workflows/ci.yml
- la/matrix_ops_test.v
- vlas/cflags_d_cblas.v
- vlas/cflags_notd_cblas.v
- vlas/lapack_common.v
Additional comments (Suppressed): 4
vlas/lapack_default.c.v (1)
- 1-9: The function
C.LAPACKE_dlangeis declared but not defined. This could lead to a linker error if the function is not defined elsewhere. Please ensure that the function is defined in the linked C libraries. Also, the functiondlangeis using unsafe code to callC.LAPACKE_dlange. While this is sometimes necessary when interfacing with C, it's important to ensure that the inputs to the function are valid to prevent undefined behavior. Specifically, make sure that the slicesaandworkare not empty before accessing their first element.float/float64/ge_test.v (2)
71-72: The tolerance for floating-point comparisons has been made conditional based on the operating system. This is likely due to differences in floating-point precision between different systems. Ensure that this change does not introduce any unexpected behavior or test failures on other systems.
90-90: The assertion error message has been enhanced to include the actual value, expected value, and tolerance. This will be helpful in debugging test failures.
iter/ranges_test.v (1)
- 3-3: The
mathmodule is imported but not used in the old hunk. Ensure that it is necessary in the new hunk.
| run: ~/.vmodules/vsl/bin/test ${{ matrix.flags }} --use-cblas --use-gc boehm | ||
| if: ${{ matrix.os != 'ubuntu-18.04' }} |
There was a problem hiding this comment.
Removes apparently obsolete
- gc boehm is the default mode, but I might be missing something that it was called explicitly here. Else, there is no difference with the run above.
- ubuntu-18.04 is not in the matrix, support for such runners was deprecated
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- iter/ranges_test.v (3 hunks)
Additional comments (Suppressed): 3
iter/ranges_test.v (3)
3-3: The
mathmodule is imported but was not present in the old hunk. This is likely to support the newmath.tolerancefunction used in the tests. Ensure that this import is necessary and that themathmodule is available in the project's dependencies.54-65: The test assertions for
FloatIterhave been updated to include a platform-specific tolerance check. On macOS, a higher tolerance of2e-15is used, while on other platforms, the originaln.eq_epsilon(expected[i][j])check is maintained. This change is likely due to floating-point precision differences on macOS. Ensure that this change does not affect the accuracy of the tests on other platforms.94-105: Similar to the
FloatItertests, the test assertions forLinearIterhave also been updated to include a platform-specific tolerance check. Ensure that this change does not affect the accuracy of the tests on other platforms.
ttytm
force-pushed
the
fix/macos-tests
branch
from
90370d7 to
345ee81
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (10)
- .github/workflows/ci.yml (2 hunks)
- float/float64/ge_test.v (2 hunks)
- iter/ranges_test.v (3 hunks)
- la/matrix_ops_test.v (1 hunks)
- vlas/cflags_d_cblas.v (1 hunks)
- vlas/cflags_notd_cblas.v (1 hunks)
- vlas/conversions.v (1 hunks)
- vlas/lapack_common.v (2 hunks)
- vlas/lapack_default.c.v (1 hunks)
- vlas/lapack_macos.c.v (1 hunks)
Files skipped from review due to trivial changes (3)
- .github/workflows/ci.yml
- vlas/cflags_d_cblas.v
- vlas/cflags_notd_cblas.v
Additional comments (Suppressed): 14
vlas/lapack_macos.c.v (1)
- 1-7: The function
dlangeis a wrapper around the C functionLAPACKE_dlange. It's important to ensure that the C function is correctly declared and that the types of the parameters match between the V and C versions of the function. Also, the use ofunsafeshould be minimized and justified. In this case, it seems necessary to call the C function, but it's worth double-checking if there's a safer alternative.vlas/lapack_common.v (3)
16-20: The function signatures for
C.LAPACKE_dgeev,C.LAPACKE_dsyev, andC.LAPACKE_dgebalhave been modified. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures. Also, verify that the changes are compatible with the LAPACK library being used.192-193: The function
C.LAPACKE_dgeevis now being called with thejob_vlrfunction applied tocalc_vlandcalc_vrand then converted to a string. Ensure that thejob_vlrfunction returns the correct values and that the LAPACK library can handle these string inputs.189-198: The error handling for the
C.LAPACKE_dgeevfunction call remains the same. If the function call fails, the program will panic with the message 'lapack failed'. This is a good practice as it immediately alerts the user to the failure.la/matrix_ops_test.v (1)
- 32-36: The new code introduces platform-specific tolerance levels for floating-point comparisons. This is a good approach to handle precision differences across platforms. However, it's important to ensure that the tolerance level
la.matrix_tests_tolis appropriately set for macOS. Also, consider usingmath.tolerancefor all platforms for consistency, as floating-point precision issues can occur on any platform, not just macOS.- $if macos { - assert math.tolerance(matrix_det(mat_b), -306, la.matrix_tests_tol) - } $else { - assert matrix_det(mat_b) == -306 - } + assert math.tolerance(matrix_det(mat_b), -306, la.matrix_tests_tol)float/float64/ge_test.v (2)
71-72: The tolerance for floating-point comparisons in the
test_gerfunction has been made platform-specific. This is a good approach to handle precision differences across platforms. However, ensure that the chosen tolerance values are appropriate for the precision of the calculations being performed on each platform.90-90: The assertion error message has been enhanced to include the actual value, expected value, and tolerance. This is a good practice as it provides more context when a test fails, making it easier to debug.
iter/ranges_test.v (3)
3-3: The
mathmodule is imported but not used in the old hunk. Ensure that it is necessary for the new hunk.54-65: The new hunk introduces platform-specific tolerance levels for floating-point comparisons in the
test_float_iter()function. This is a good approach to account for precision differences across platforms. However, the tolerance for macOS is set to2e-15, which is quite high. If this value is not carefully chosen, it could lead to false positives in the tests. Please verify that this tolerance level is appropriate for the precision of the calculations being tested.94-105: Similar to the previous comment, the new hunk introduces platform-specific tolerance levels for floating-point comparisons in the
test_linear_iter()function. The same considerations apply: ensure that the tolerance level is appropriate for the precision of the calculations being tested.vlas/conversions.v (4)
9-11: The refactoring from if-else statements to conditional expressions improves readability and conciseness of the code. This change is consistent with the best practices for the V language.
13-15: The refactoring from if-else statements to conditional expressions improves readability and conciseness of the code. This change is consistent with the best practices for the V language.
17-19: The refactoring from if-else statements to conditional expressions improves readability and conciseness of the code. This change is consistent with the best practices for the V language.
21-23: The return type of
job_vlrfunction has been changed from&chartorune. Ensure that this change does not break any existing code that relies on the old return type. Also, the refactoring from if-else statements to conditional expressions improves readability and conciseness of the code.
| module vlas | ||
|
|
||
| import vsl.vlas.internal.vblas | ||
|
|
||
| fn C.LAPACKE_dlange(matrix_layout vblas.MemoryLayout, norm &char, m int, n int, a &f64, lda int, work &f64) f64 | ||
|
|
||
| pub fn dlange(norm rune, m int, n int, a []f64, lda int, work []f64) f64 { | ||
| return unsafe { | ||
| C.LAPACKE_dlange(.row_major, &char(norm.str().str), m, n, &a[0], lda, &work[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function dlange is a wrapper around the C function LAPACKE_dlange. It's good to see that you're using the unsafe block to call the C function, which is a good practice in V language when dealing with C interop. However, it's important to ensure that the parameters passed to the C function are valid and won't cause any undefined behavior. For instance, you're passing the address of the first element of the a and work arrays to the C function. If these arrays are empty, this will result in undefined behavior. You should add a check to ensure these arrays are not empty before calling the C function.
pub fn dlange(norm rune, m int, n int, a []f64, lda int, work []f64) f64 {
+ if a.len == 0 || work.len == 0 {
+ return 0.0
+ }
return unsafe {
C.LAPACKE_dlange(.row_major, &char(norm.str().str), m, n, &a[0], lda, &work[0])
}
}
| - name: Execute Tests using Pure V Backend with Pure C Blas and Garbage Collection enabled | ||
| continue-on-error: true | ||
| run: ~/.vmodules/vsl/bin/test --use-cblas --use-gc boehm | ||
| run: ~/.vmodules/vsl/bin/test --use-cblas --skip-examples |
There was a problem hiding this comment.
why are we using the flag --skip-examples? 👀
There was a problem hiding this comment.
Tests work, but not all examples build on mac currently.
Thought we can re-enable it when they do.
Best #182 first, then C types can be updated here as well before a merge.
Apparently math works different on mac, needed to add a little tolarance for some tests.
Summary by CodeRabbit